Audio modules fixes by lgirdwood · Pull Request #10752 · thesofproject/sof

lgirdwood · 2026-05-07T19:40:38Z

Some small fixes picked up by the audio ztest UTs.

Properly track allocation state in the ASRC component lifecycle to prevent double-free of heap memory during module teardown in ZTest environments. Signed-off-by: Liam Girdwood <[email protected]>

Add input validation for the frame_shift configuration field in mfcc_setup(). A zero or negative value would cause a division by zero during STFT processing. Signed-off-by: Liam Girdwood <[email protected]>

Add null checks for hardware device pointers that may not be available when running in ZTest environments without real hardware. Signed-off-by: Liam Girdwood <[email protected]>

Copilot

Pull request overview

This PR addresses a few robustness issues in the audio modules that were surfaced by the audio ztest unit tests, primarily by adding missing validation and making teardown paths safer.

Changes:

Add Zephyr device_is_ready() gating to mic privacy manager initialization and guard policy queries when the device isn’t initialized.
Add MFCC configuration validation to reject non-positive frame_shift.
Harden ASRC free/reset paths by adding NULL checks, clearing pointers after freeing, and resetting module private data.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File	Description
`src/audio/mic_privacy_manager/mic_privacy_manager_intel.c`	Add device readiness check during init and a guard in `get_policy()` to avoid dereferencing an uninitialized device pointer.
`src/audio/mfcc/mfcc_setup.c`	Add basic validation for `frame_shift` to prevent invalid hop sizes.
`src/audio/asrc/asrc.c`	Make `free()`/`reset()` more defensive by checking allocations before releasing and nulling pointers after free.

kv2019i · 2026-05-11T10:45:06Z

+	if (!device_is_ready(mic_priv_dev)) {
+		LOG_ERR("mic_privacy device not ready");
+		return -ENODEV;
+	}


This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?

probably want to drop it for native_sim, but for qemu ACE its a doable test - the funny thing is copilot is reviewing itself here for this change.

lgirdwood · 2026-05-13T15:29:18Z

 	fft->fft_size = config->frame_length;
 	fft->fft_padded_size = 1 << (31 - norm_int32(fft->fft_size)); /* Round up to nearest 2^N */
+	if (config->frame_shift <= 0) {
+		comp_err(dev, "frame_shift must be positive");
+		return -EINVAL;
+	}


@singalsu fyi - these came up as crashes during the ztest UT as it was passing in zeros for stuff it did not know about. Will get this fixed.

kv2019i

Looks ok. Not 100% sure whether for mic privacy would be easier to just disable mic privacy for ztest runs. Otherwise you'd need add more checks (or make the dev null if init is not done).

kv2019i · 2026-05-11T10:45:06Z

+	if (!device_is_ready(mic_priv_dev)) {
+		LOG_ERR("mic_privacy device not ready");
+		return -ENODEV;
+	}


This is a valid concern. Maybe alternative is to drop mic privacy when running the ztest?

lyakh

please don't re-add function names to prints. As for adding all the checks, IIUC they can only trigger in ztests, right? Do we really want to add code to actual run-time paths to satisfy our ztests or should we rather adjust ztests to avoid impossible situations? I don't think adding such checks for our test flows is scalable - we can call all (non-static) functions with invalid arguments. Should we add checks everywhere for them?

lyakh · 2026-05-12T08:20:36Z

-	struct comp_dev *dev = mod->dev;

-	comp_dbg(dev, "entry");
+	comp_info(mod->dev, "asrc_free() entry");


we have removed all function names from all logging on purpose, let's not start re-adding them. Function names are printed automatically by Zephyr.

lyakh · 2026-05-12T08:22:43Z

+	comp_info(mod->dev, "asrc_free() entry");
+
+	if (!cd)
+		return 0;


can it be called with cd == NULL? Don't think so, .free() is probably only called when .init() was successful

lyakh · 2026-05-12T08:23:51Z

-	mod_free(mod, cd->asrc_obj);
+	cd->buf = NULL;
+
+	if (cd->asrc_obj) {


same, can it be NULL here?

lyakh · 2026-05-12T08:25:37Z

+		cd->asrc_obj = NULL;
+	}
+
+	if (cd->buf) {


same for the above 2 added checks

lyakh · 2026-05-12T08:28:04Z

 	state->emph.coef = -config->preemphasis_coefficient; /* Negate config parameter */
 	fft->fft_size = config->frame_length;
 	fft->fft_padded_size = 1 << (31 - norm_int32(fft->fft_size)); /* Round up to nearest 2^N */
+	if (config->frame_shift <= 0) {


can this be hit in real pipelines or only in ztests?

lgirdwood · 2026-05-13T15:40:06Z

please don't re-add function names to prints. As for adding all the checks, IIUC they can only trigger in ztests, right? Do we really want to add code to actual run-time paths to satisfy our ztests or should we rather adjust ztests to avoid impossible situations? I don't think adding such checks for our test flows is scalable - we can call all (non-static) functions with invalid arguments. Should we add checks everywhere for them?

copilot fixed and added __func__, but this really means we have to adopt LOG() everywhere so that agents dont repeat.

We do need individual module UT ztests as we have no way to test in isolation, yes some of the data being passed in is coming out of band and not using normal runtime paths, but this is important as its obvious and now easy next step is to connect the modules APIs to fuzzing output in sim_native.

lrgirdwo added 3 commits May 6, 2026 16:54

audio: asrc: Fix heap double-free during teardown

7a514af

Properly track allocation state in the ASRC component lifecycle to prevent double-free of heap memory during module teardown in ZTest environments. Signed-off-by: Liam Girdwood <[email protected]>

audio: mfcc: Add frame_shift validation to prevent division by zero

2080969

Add input validation for the frame_shift configuration field in mfcc_setup(). A zero or negative value would cause a division by zero during STFT processing. Signed-off-by: Liam Girdwood <[email protected]>

audio: mic_privacy_manager: Fix NULL pointer dereference in ZTest

1ebfd38

Add null checks for hardware device pointers that may not be available when running in ZTest environments without real hardware. Signed-off-by: Liam Girdwood <[email protected]>

Copilot AI review requested due to automatic review settings May 7, 2026 19:40

lgirdwood requested review from dbaluta, kv2019i, lbetlej, mmaka1 and plbossart as code owners May 7, 2026 19:40

Copilot started reviewing on behalf of lgirdwood May 7, 2026 19:43 View session

Copilot AI reviewed May 7, 2026

View reviewed changes

kv2019i reviewed May 11, 2026

View reviewed changes

lyakh requested changes May 12, 2026

View reviewed changes

Conversation

lgirdwood commented May 7, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kv2019i left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lyakh left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lgirdwood commented May 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants